-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding repace_table() #11
base: master
Are you sure you want to change the base?
Conversation
@@ -42,14 +42,57 @@ def replace_picture(self, label: str, filename: _Pathlike) -> None: | |||
""" | |||
pass | |||
|
|||
def replace_table(self, label: str, data) -> None: | |||
def replace_table(self, label: str, data, header=False, rownames=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We allow use kwargs only for header
and rownames
:
def replace_table(self, label: str, data, *, header=False, rownames=False):
In my opinion it's better to use None
instead of False
. When using False
as default value I assume a Boolean value is required. None
is no valid data fore these arguments and therefore it should be fine to use it. Using an Undefined()
object could be an option, too. But I think this is not necessary in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor comment for _find_shapes
. Other that that, I think, tests should definitely be included before merging this.
pass | ||
assert isinstance(data, pandas.dataframe) | ||
|
||
shapes_to_replace = self._find_shapes(label) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In #7, the method signature of _find_shapes
was changed? Maybe change this to reflect that?
"""Replaces rectangle placeholders on one or many slides. | ||
|
||
Args: | ||
label (str): label of the placeholder (without curly braces) | ||
data (pandas.DataFrame): table to be inserted into the presentation | ||
""" | ||
pass | ||
assert isinstance(data, pandas.dataframe) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could remove the assertion here and include type hints in the method signature like so:
def replace_table(self, label: str, data: pandas.dataframe, *, header: bool = False, rownames: bool = False) -> None:
Hi,
Adding my replace_table() from yesterday. Tests not yet created.
Best,
Martin